Repository navigation
GH-54533: [C++][Parquet] Avoid emitting empty data pages and advance past empty pages in reader - #54582
Open
ShivanshhhG wants to merge 1 commit into
Open
GH-54533: [C++][Parquet] Avoid emitting empty data pages and advance past empty pages in reader#54582ShivanshhhG wants to merge 1 commit into
ShivanshhhG wants to merge 1 commit into
Conversation
…vance past empty pages in reader When writing a repeated column with a single record batch or a small data_page_size, the writer could check page limits when num_buffered_values_ is zero. Because empty encoders (such as RleBooleanEncoder or DictionaryEncoder) estimate a small non-zero encoded size, the writer previously emitted an empty first data page (num_values = 0). On read, HasNextInternal returned false upon encountering the empty page, prematurely truncating the column chunk and silently dropping the row group's rows. This commit: 1. In CommitWriteAndCheckPageLimit, ensures num_buffered_values_ > 0 before triggering AddDataPage(). 2. In ColumnWriterImpl::AddDataPage(), guards against adding empty pages when num_buffered_values_ == 0. 3. In ColumnReaderImplBase::HasNextInternal(), loops across any data page with zero buffered values rather than treating it as end of column chunk, ensuring robust backward compatibility with existing Parquet files. 4. Adds unit tests in arrow_reader_writer_test.cc verifying repeated columns with small page sizes, V2 pages, dictionary encoding, and multi-column schemas. Closes: apache#54533 Signed-off-by: SHIVANSH-ux-ys <singaser78@gmail.com>
|
|
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
When writing a repeated column with a single record or a small
data_page_size, the Parquet writer could evaluate page limits whennum_buffered_values_ == 0. Because empty encoders (such asRleBooleanEncoderand dictionary encoders) return a small non-zero estimated size (e.g. 69 bytes or 6 bytes),EstimatedDataEncodedSize() >= data_pagesize()evaluated to true andAddDataPage()was invoked, writing an empty data page (num_values = 0,num_rows = 0) at the start of the column chunk.On read,
ColumnReaderImplBase::HasNextInternal()checkedif (!ReadNewPage() || num_buffered_values_ == 0)and returnedfalse, erroneously treating the empty data page as end-of-column-chunk and silently dropping all rows in that row group.What changes are included in this PR?
TypedColumnWriterImpl::CommitWriteAndCheckPageLimit: requirednum_buffered_values_ > 0before checking page limit conditions and triggeringAddDataPage().ColumnWriterImpl::AddDataPage(): added an early-return guardif (num_buffered_values_ == 0) return;to prevent writing empty data pages.ColumnReaderImplBase::HasNextInternal(): converted the page check to awhile (num_buffered_values_ == 0 || num_decoded_values_ == num_buffered_values_)loop so that any data page with zero values is skipped, advancing to subsequent pages rather than prematurely terminating read. This provides backward compatibility for existing Parquet files.cpp/src/parquet/arrow/arrow_reader_writer_test.cc:SingleRowRepeatedSmallPageSize: single-row repeated column with smalldata_page_sizeand V2 data pages.LastRowGroupSingleRowRepeatedSmallPageSize: repeated columns across multiple row groups where the last row group has 1 row.ListDictionarySmallPageSize: repeated columns with dictionary encoding and small page size.MultiColumnRepeatedSmallPageSize: multi-column table with a repeated column.Are these changes tested?
Yes, new unit tests covering repeated columns with small page sizes and dictionary encoding have been added to
arrow_reader_writer_test.cc.Are there any user-facing changes?
No breaking changes. This fixes a silent data loss bug where repeated columns with small page sizes or single-row batches had rows dropped.
This PR contains a "Critical Fix".
Fixes a bug that caused rows in Parquet files with repeated columns to be silently missing on read.
Was AI used for this PR?
PR code and description written by:
Reviewed before submission by:
Closes #54533.